Refactor Channel creation #377

fabianfett · 2021-06-16T21:25:32Z

This is the first cherry pick from #376.

The connection creation logic has been refactored into a number of smaller methods that can be combined for the configuration.
Connection creation now has a logical home. We move it from Utils.swift into a ConnectionFactory
We have explicit ChannelHandlers that we use for connection creation:
- TLSEventsHandler got its own file and unit tests
- HTTP1ProxyConnectHandler got its own file and unit tests
Some small things are already part of this pr that will get their context later. For example:
- HTTPConnectionPool is added as a namespace to not cause major renames in follow up PRs
- HTTPConnectionPool.Connection.ID and its generator were added now. (This will be used later to identify a connection during its lifetime)
- the file HTTPConnectionPool+Manager was added to give HTTPConnectionPool.Connection.ID.Generator already its final destination.

Looking forward to your feedback.
I'm well aware that this PR is in a race with #375. I'm happy to update it once #375 has landed.

fabianfett · 2021-06-16T22:02:32Z

Ran into a test failure in Swift 5.1: testSSLHandshakeErrorPropagation failed.

Looks like this one used to be flaky. But I assume @Lukasa fixed it with #335. Investigated the test's flakyness with 100 iterations in Xcode (all passed). Will need further investigation on Linux.

Sources/AsyncHTTPClient/ConnectionPool/HTTP1ProxyConnectHandler.swift

Sources/AsyncHTTPClient/ConnectionPool/HTTPConnectionPool+Factory.swift

artemredkin · 2021-06-21T10:28:42Z

Sources/AsyncHTTPClient/ConnectionPool/HTTPConnectionPool+Manager.swift

+    static var globalGenerator = Generator()
+
+    struct Generator {
+        private let atomic: NIOAtomic<Int>


maybe we can make it UInt64, just in case someone will be running it until our sun dies 😆

artemredkin

Some minor suggestions, but otherwise looks good 👍

Sources/AsyncHTTPClient/ConnectionPool.swift

Sources/AsyncHTTPClient/ConnectionPool/HTTP1ProxyConnectHandler.swift

Sources/AsyncHTTPClient/ConnectionPool/HTTPConnectionPool+Factory.swift

Davidde94 · 2021-06-21T11:16:00Z

Sources/AsyncHTTPClient/ConnectionPool/HTTPConnectionPool+Factory.swift

+    var isIPAddress: Bool {
+        var ipv4Addr = in_addr()
+        var ipv6Addr = in6_addr()
+


Suggested change

Sources/AsyncHTTPClient/HTTPClient+Proxy.swift

Tests/AsyncHTTPClientTests/HTTP1ProxyConnectHandlerTests.swift

Sources/AsyncHTTPClient/ConnectionPool/HTTP1ProxyConnectHandler.swift

Sources/AsyncHTTPClient/ConnectionPool/HTTPConnectionPool+Factory.swift

Package.swift

Sources/AsyncHTTPClient/ConnectionPool/ChannelHandler/SOCKSEventsHandler.swift

Sources/AsyncHTTPClient/ConnectionPool/HTTPConnectionPool+Factory.swift

Davidde94 · 2021-06-22T18:09:36Z

Sources/AsyncHTTPClient/ConnectionPool/HTTPConnectionPool+Factory.swift

+        #if canImport(Network)
+            if #available(OSX 10.14, iOS 12.0, tvOS 12.0, watchOS 6.0, *), let tsBootstrap = NIOTSConnectionBootstrap(validatingGroup: eventLoop) {
+                // create NIOClientTCPBootstrap with NIOTS TLS provider
+                let bootstrapFuture = tlsConfig.getNWProtocolTLSOptions(on: eventLoop).map {
+                    options -> NIOClientTCPBootstrapProtocol in
+
+                    tsBootstrap
+                        .addTimeoutIfNeeded(self.clientConfiguration.timeout)
+                        .tlsOptions(options)
+                        .channelInitializer { channel in
+                            do {
+                                try channel.pipeline.syncOperations.addHandler(HTTPClient.NWErrorHandler())
+                                // we don't need to set a TLS deadline for NIOTS connections, since the
+                                // TLS handshake is part of the TS connection bootstrap. If the TLS
+                                // handshake times out the complete connection creation will be failed.
+                                try channel.pipeline.syncOperations.addHandler(TLSEventsHandler(deadline: nil))
+                                return channel.eventLoop.makeSucceededFuture(())
+                            } catch {
+                                return channel.eventLoop.makeFailedFuture(error)
+                            }
+                        } as NIOClientTCPBootstrapProtocol
+                }
+                return bootstrapFuture
+            }
+        #endif


This section is kind of rough to read, could it be refactored a little to have less indentation?

Davidde94 · 2021-06-22T18:10:00Z

Sources/AsyncHTTPClient/ConnectionPool/HTTPConnectionPool+Factory.swift

+        guard let connectTimeamount = config?.connect else {
+            return self
+        }
+


Suggested change

Sources/AsyncHTTPClient/HTTPClient+Proxy.swift

Co-authored-by: Cory Benfield <[email protected]>

Davidde94 · 2021-06-28T09:18:06Z

Sources/AsyncHTTPClient/ConnectionPool/ChannelHandler/HTTP1ProxyConnectHandler.swift

+
+    func handlerAdded(context: ChannelHandlerContext) {
+        self.proxyEstablishedPromise = context.eventLoop.makePromise(of: Void.self)
+


Davidde94 · 2021-06-28T09:18:14Z

Sources/AsyncHTTPClient/ConnectionPool/ChannelHandler/HTTP1ProxyConnectHandler.swift

+        case .connectSent(let timeout), .headReceived(let timeout):
+            timeout.cancel()
+            self.failWithError(HTTPClientError.remoteConnectionClosed, context: context, closeConnection: false)
+


Suggested change

Davidde94 · 2021-06-28T09:18:30Z

Sources/AsyncHTTPClient/ConnectionPool/ChannelHandler/HTTP1ProxyConnectHandler.swift

+
+            case .connectSent, .headReceived:
+                self.failWithError(HTTPClientError.httpProxyHandshakeTimeout, context: context)
+


Suggested change

Davidde94 · 2021-06-28T09:18:34Z

Sources/AsyncHTTPClient/ConnectionPool/ChannelHandler/HTTP1ProxyConnectHandler.swift

+            switch self.state {
+            case .initialized:
+                preconditionFailure("How can we have a scheduled timeout, if the connection is not even up?")
+


Suggested change

Davidde94 · 2021-06-28T09:18:54Z

Sources/AsyncHTTPClient/ConnectionPool/ChannelHandler/HTTP1ProxyConnectHandler.swift

+            self.state = .headReceived(scheduled)
+        case 407:
+            self.failWithError(HTTPClientError.proxyAuthenticationRequired, context: context)
+


Suggested change

Davidde94 · 2021-06-28T09:19:04Z

Sources/AsyncHTTPClient/ConnectionPool/ChannelHandler/HTTP1ProxyConnectHandler.swift

+            timeout.cancel()
+            self.state = .completed
+            self.proxyEstablishedPromise?.succeed(())
+


Suggested change

Davidde94 · 2021-06-28T09:20:01Z

Sources/AsyncHTTPClient/ConnectionPool/ChannelHandler/SOCKSEventsHandler.swift

+
+    func handlerAdded(context: ChannelHandlerContext) {
+        self.socksEstablishedPromise = context.eventLoop.makePromise(of: Void.self)
+


Suggested change

Davidde94 · 2021-06-28T09:22:30Z

Sources/AsyncHTTPClient/ConnectionPool/HTTPConnectionPool+Factory.swift

+        case .https, .https_unix:
+            return self.makeTLSChannel(deadline: deadline, eventLoop: eventLoop, logger: logger).flatMapThrowing {
+                channel, negotiated in
+


Suggested change

Sources/AsyncHTTPClient/ConnectionPool.swift

Lukasa

LGTM.

fabianfett requested review from Lukasa, Davidde94 and PeterAdams-A June 16, 2021 21:25

fabianfett force-pushed the ff-channel-creation branch 5 times, most recently from b97d82c to 0a6bef5 Compare June 16, 2021 21:46

fabianfett requested a review from artemredkin June 16, 2021 22:02

Lukasa added the 🔨 semver/patch No public API change. label Jun 17, 2021

artemredkin reviewed Jun 21, 2021

View reviewed changes

Sources/AsyncHTTPClient/ConnectionPool/HTTP1ProxyConnectHandler.swift Outdated Show resolved Hide resolved

artemredkin reviewed Jun 21, 2021

View reviewed changes

Sources/AsyncHTTPClient/ConnectionPool/HTTPConnectionPool+Factory.swift Show resolved Hide resolved

artemredkin reviewed Jun 21, 2021

View reviewed changes

Sources/AsyncHTTPClient/ConnectionPool/HTTPConnectionPool+Factory.swift Show resolved Hide resolved

artemredkin reviewed Jun 21, 2021

View reviewed changes

artemredkin approved these changes Jun 21, 2021

View reviewed changes

Davidde94 reviewed Jun 21, 2021

View reviewed changes

fabianfett force-pushed the ff-channel-creation branch 5 times, most recently from 1b08850 to ed91313 Compare June 21, 2021 14:42

Lukasa requested changes Jun 21, 2021

View reviewed changes

Davidde94 reviewed Jun 22, 2021

View reviewed changes

fabianfett force-pushed the ff-channel-creation branch 3 times, most recently from f4ce479 to 638bce9 Compare June 23, 2021 08:14

fabianfett requested review from Lukasa, Davidde94 and glbrntt June 23, 2021 09:00

fabianfett force-pushed the ff-channel-creation branch from 10f24b9 to 1353129 Compare June 23, 2021 14:54

fabianfett requested review from Davidde94 and Lukasa June 23, 2021 14:55

fabianfett force-pushed the ff-channel-creation branch from 1353129 to f92293f Compare June 23, 2021 14:59

fabianfett added this to the 1.5.0 milestone Jun 23, 2021

fabianfett and others added 15 commits June 25, 2021 12:16

Channel creation refactored

5601563

First round of reviews

682f65a

Code review comments

4c08e8f

Code review

d4b9367

Timeout for TLS and SOCKS Handshake

27f6c70

SOCKS, HTTPProxy and TLS are now protected against timeouts

8f243f6

Speed up tests.

6fb9481

Code review

b2c371e

Code review

101f8b9

Code review: We use a deadline now for connection creation.

f9d1901

swiftformat

b443228

Apply suggestions from code review

ee6a452

Co-authored-by: Cory Benfield <[email protected]>

Code review

390a790

Make testSSLHandshakeErrorPropagationDelayedClose less flaky

7ba6a96

Factory is a struct

7aec48b

fabianfett force-pushed the ff-channel-creation branch from b489196 to 7aec48b Compare June 25, 2021 10:16

makeSucceededFuture(()) -> makeSucceededVoidFuture()

b58bc15

Davidde94 approved these changes Jun 28, 2021

View reviewed changes

Lukasa reviewed Jun 28, 2021

View reviewed changes

Sources/AsyncHTTPClient/ConnectionPool.swift Outdated Show resolved Hide resolved

Sources/AsyncHTTPClient/ConnectionPool.swift Outdated Show resolved Hide resolved

fabianfett added 2 commits June 28, 2021 12:15

Code review: Code moved.

6cfd62c

Code review: Add internal connectionCreationTimeout

761b699

fabianfett force-pushed the ff-channel-creation branch from 40d73e2 to 761b699 Compare June 28, 2021 10:22

Lukasa approved these changes Jun 28, 2021

View reviewed changes

fabianfett merged commit 8e4d519 into swift-server:main Jun 28, 2021

fabianfett deleted the ff-channel-creation branch June 28, 2021 11:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Channel creation #377

Refactor Channel creation #377

fabianfett commented Jun 16, 2021

fabianfett commented Jun 16, 2021

artemredkin Jun 21, 2021

artemredkin left a comment

Davidde94 Jun 21, 2021

Davidde94 Jun 22, 2021

Davidde94 Jun 22, 2021

Davidde94 Jun 28, 2021

Davidde94 Jun 28, 2021

Davidde94 Jun 28, 2021

Davidde94 Jun 28, 2021

Davidde94 Jun 28, 2021

Davidde94 Jun 28, 2021

Davidde94 Jun 28, 2021

Davidde94 Jun 28, 2021

Lukasa left a comment


		func handlerAdded(context: ChannelHandlerContext) {
		self.proxyEstablishedPromise = context.eventLoop.makePromise(of: Void.self)


		case .connectSent, .headReceived:
		self.failWithError(HTTPClientError.httpProxyHandshakeTimeout, context: context)


		func handlerAdded(context: ChannelHandlerContext) {
		self.socksEstablishedPromise = context.eventLoop.makePromise(of: Void.self)

Refactor Channel creation #377

Refactor Channel creation #377

Conversation

fabianfett commented Jun 16, 2021

fabianfett commented Jun 16, 2021

Choose a reason for hiding this comment

artemredkin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lukasa left a comment

Choose a reason for hiding this comment